Attempt to make acquireSearcher lighter for can match #126140
Conversation
| ) { | ||
| super(reader); | ||
| // pass in an executor so we can shortcut and avoid looping through all segments | ||
| super(reader, source.equals(CAN_MATCH_SEARCH_SOURCE) ? Runnable::run : null); |
There was a problem hiding this comment.
there's many more scenarios where all we need is an index reader. This is quite hard to untangle due to the many acquireSearcher callers and different variants of the method, wrapping going on as well. I am tempted to add an acquireIndexReader method to Engine, but this requires significant cleanup to become effective and better than what we currently have.
There was a problem hiding this comment.
On the other hand, if there is significant overhead on creating a searcher, perhaps that's something to fix in Lucene rather than working around in ES. What triggered this change is suspicions around overhead caused by creating slices eagerly, mostly stuff that happens in the IndexSearcher constructor. We should figure out how to minimize that in Lucene?
There was a problem hiding this comment.
I agree and apache/lucene#14343 should help quite a bit on this front. I wonder though, maybe the problem isn't so much with the slice cost but with the fact that we don't really reuse searcher instances much. We don't really need a new searcher (for e.g. can_match) across queries so long as we don't see new data do we?
There was a problem hiding this comment.
That is definitely a possible improvement, but what I am trying to do is figure out the cause of a regression. We have not changed how many times we create a searcher, but we did add overhead to the creation of a searcher.
javanna
changed the title
javanna
changed the title
javanna
added
:Search Foundations/Search
elasticsearchmachine
added
the
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
| } | ||
|
|
||
| @Override | ||
| protected void searchLeaf(LeafReaderContext ctx, int minDocId, int maxDocId, Weight weight, Collector collector) |
There was a problem hiding this comment.
Just curious - why did you add the assertion in this method?
There was a problem hiding this comment.
because I want to make sure that when we shortcut the slicing, searchLeaf is never called. That's the assumption based on which we can shortcut the slices to an empty array, because a search will never be executed on top of this searcher instance.
benchaplin
left a comment
benchaplin
left a comment
There was a problem hiding this comment.
In case you still want to merge this!
4 participants
There seems to be some overhead observed in some of our benchmarks around creating an
IndexSearcher. In many places, like can match, we acquire a searcher but all we need is an index reader. This small change tries to verify some assumptions by removing the slight slicing overhead caused by looping over the segments when we create a searcher without providing an executor to its constructor. We may iterate on this approach alter, but first I'd like to see the impact of this on our nightly benchmarks.